Replace Travis with Github Actions for CI builds #6071

mjjbell · 2021-07-04T18:52:57Z

Issue

This PR replaces Travis for continuous integration with Github Actions.

The Github Actions pipeline is functionally equivalent, with all the same build permutations supported. Whilst the Github Actions offering is broadly equivalent to Travis, a few changes have been made as part of the migration.

The core and optional Travis stages have been consolidated into one build matrix. This is due to the current inability in
Github Actions to share build steps between jobs, so this avoids having to duplicate the steps.
Optional stage jobs will now run in parallel with core jobs, but they still remain optional in the sense that they don't fail the build.

A number of existing Github Action plugins are used to replace functionality provided by Travis or other tools:

Linux builds are updated to build on Ubuntu 18.04. MacOS builds are updated to run on 10.15.
Similar to the Travis Xenial upgrade attempt, some changes are required due to underlying platform and compiler upgrades. This means some Node 10 toolchains will no longer be supported.

Whilst there is opportunity to upgrade build dependencies and make the CI steps more idiomatic, I've left this for future changes and just focussed on functional replication.

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Closes #6056

mjjbell · 2021-07-04T19:00:15Z

CMakeLists.txt

@@ -514,7 +514,7 @@ if(ENABLE_MASON)
  set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${LINKER_FLAGS}")

  # current mason packages target -D_GLIBCXX_USE_CXX11_ABI=0
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_GLIBCXX_USE_CXX11_ABI=0")
+  add_dependency_defines(-D_GLIBCXX_USE_CXX11_ABI=0)


On Travis Trusty builds, the compilers all had GLIBCXX_USE_CXX11_ABI=0 by default. This setting has been flipped in Bionic, so we need to explicitly pass this to pkg-config so that the example builds.

I forgot why we had this in the first place. I think this was mason related given that is afaik the only place where we care about ABI of our binaries.

Yep, the Mason Boosts are built against the old ABI .

mjjbell · 2021-07-04T19:03:19Z

src/extractor/restriction_parser.cpp

@@ -244,14 +242,15 @@ bool RestrictionParser::ShouldIgnoreRestriction(const std::string &except_tag_st

    // Be warned, this is quadratic work here, but we assume that
    // only a few exceptions are actually defined.
-    std::vector<std::string> exceptions;
-    boost::algorithm::split_regex(exceptions, except_tag_string, boost::regex("[;][ ]*"));
+    const std::regex delimiter_re("[;][ ]*");


Similar problem to the Travis Xenial upgrade attempt.
Boost.Regex and GCC5 appears to be buggy with respect to [abi:cxx11] tags, so using this as an opportunity to replace Boost.Regex with std::regex.

mjjbell · 2021-07-04T19:04:38Z

unit_tests/library/json.cpp

@@ -27,7 +26,7 @@ BOOST_AUTO_TEST_CASE(test_json_linestring)
    const auto coords = geom.values["coordinates"].get<util::json::Array>().values;
    BOOST_CHECK_EQUAL(coords.size(), 3); // array of three location arrays

-    for (const auto each : coords)
+    for (const auto &each : coords)


Unit test changes are due to compiler errors from updated macOS clang.

This crashes clang?

Running make tests with clang 12

error: loop variable 'each' creates a copy from type 'const mapbox::util::variant<osrm::util::json::String, osrm::util::json::Number, mapbox::util::recursive_wrapperosrm::util::json::Object, mapbox::util::recursive_wrapperosrm::util::json::Array, osrm::util::json::True, osrm::util::json::False, osrm::util::json::Null>' [-Werror,-Wrange-loop-construct]
for (const auto each : coords)

Ah I see -Werror strikes again.

mjjbell · 2021-07-08T21:56:40Z

It looks like Github Actions makes each job have a status check, rather than the action as a whole.

This would be a problem if we want checks to be required because any change to the job matrix would break the required list.

I think a solution is to have a no-op job that depends on all other jobs succeeding, and make this the only required check. That way we get required stability as the jobs evolve.

mjjbell · 2021-07-08T23:35:55Z

I think a solution is to have a no-op job that depends on all other jobs succeeding, and make this the only required check. That way we get required stability as the jobs evolve.

Added a ci-complete step that can be used as a required check.

mjjbell · 2021-07-08T23:51:03Z

@danpat any thoughts on this?
Given the Travis integration is dead but required, we can't merge anything until it is disabled/replaced.

TheMarex · 2021-09-01T11:13:34Z

Added myself as reviewer, I'll take a look Friday.

TheMarex

Looks great, thanks for doing this. I think we can also trim the matrix a bit, there is little sense in supporting the older GCC from 4-5 years ago.

TheMarex · 2021-09-03T09:01:01Z

src/extractor/restriction_parser.cpp

@@ -244,14 +242,15 @@ bool RestrictionParser::ShouldIgnoreRestriction(const std::string &except_tag_st

    // Be warned, this is quadratic work here, but we assume that
    // only a few exceptions are actually defined.
-    std::vector<std::string> exceptions;
-    boost::algorithm::split_regex(exceptions, except_tag_string, boost::regex("[;][ ]*"));
+    const std::regex delimiter_re("[;][ ]*");


TheMarex · 2021-09-03T09:01:34Z

unit_tests/library/json.cpp

@@ -27,7 +26,7 @@ BOOST_AUTO_TEST_CASE(test_json_linestring)
    const auto coords = geom.values["coordinates"].get<util::json::Array>().values;
    BOOST_CHECK_EQUAL(coords.size(), 3); // array of three location arrays

-    for (const auto each : coords)
+    for (const auto &each : coords)


This crashes clang?

TheMarex · 2021-09-03T09:02:22Z

unit_tests/library/match.cpp

@@ -184,7 +184,7 @@ BOOST_AUTO_TEST_CASE(test_match_fb_serialization)
    const auto matchings = fb->routes();
    const auto &number_of_matchings = matchings->size();

-    for (const auto &waypoint : *waypoints)
+    for (const auto waypoint : *waypoints)


Also clang-related?

Yes, similar errors to above: https://github.com/mjjbell/osrm-backend/runs/2957246502?check_suite_focus=true#step:15:2784

error: loop variable 'destination' is always a copy because the range of type 'const flatbuffers::Vector<flatbuffers::Offsetosrm::engine::api::fbresult::Waypoint >' does not return a reference [-Werror,-Wrange-loop-analysis]
for (const auto &destination : *destinations_array)
^
/Users/runner/work/osrm-backend/osrm-backend/unit_tests/library/table.cpp:335:10: note: use non-reference type 'const osrm::engine::api::fbresult::Waypoint *'
for (const auto &destination : *destinations_array)

TheMarex · 2021-09-03T09:03:45Z

.github/workflows/osrm-backend.yml

+            CLANG_VERSION: 5.0.0
+            ENABLE_MASON: ON
+
+          - name: gcc-9-release


I think we can simplify the build matrix a bit and deprecate the older GCCs.

mjjbell · 2021-09-03T11:29:20Z

Thanks for the review @TheMarex!

Replace Travis for continuous integration with Github Actions. The Github Actions pipeline is functionally equivalent, with all the same build permutations supported. Whilst the Github Actions offering is broadly equivalent to Travis, a few changes have been made as part of the migration. - The 'core' and 'optional' Travis stages have been consolidated into one build matrix. This is due to the current inability in Github Actions to share build steps between jobs, so this avoids having to duplicate the steps. Optional stage jobs will now run in parallel with core jobs, but they still remain optional in the sense that they don't fail the build. - A number of existing Github Action plugins are used to replace functionality provided by Travis or other tools: Node setup, caching, Codecov, publishing release artifacts. - Linux builds are updated to build on Ubuntu 18.04. MacOS builds are updated to run on 10.15. Similar to the Travis Xenial upgrade attempt, some changes are required due to underlying platform and compiler upgrades. This means some Node 10 toolchains will no longer be supported. Whilst there is opportunity to upgrade some dependencies and make the CI steps more idiomatic, I've left this for future changes and just focussed on functional replication.

mjjbell · 2021-09-03T12:34:44Z

@TheMarex would you be able to disable Travis CI as a required check? I believe only repo owners have the privileges needed.

It's configured in the Settings page: https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

The ci-complete step from Github Actions can be used as a required replacement.

TheMarex · 2021-09-03T16:34:54Z

@mjjbell Done. I'll merge this now to enable the required ci-complete step.

mjjbell mentioned this pull request Jul 4, 2021

Replace Travis with Github Actions for CI builds #6070

Closed

6 tasks

mjjbell force-pushed the mbell/replace_travis_ga branch from fd52c15 to 93e5cc8 Compare July 4, 2021 18:56

mjjbell commented Jul 4, 2021

View reviewed changes

mjjbell force-pushed the mbell/replace_travis_ga branch from 93e5cc8 to 4784e43 Compare July 4, 2021 21:27

mjjbell force-pushed the mbell/replace_travis_ga branch 3 times, most recently from 15a910a to 415bd81 Compare July 8, 2021 22:14

mjjbell mentioned this pull request Aug 28, 2021

Fix the URL parser for special chars in profile #6090

Merged

5 tasks

TheMarex self-requested a review September 1, 2021 11:13

TheMarex approved these changes Sep 3, 2021

View reviewed changes

mjjbell force-pushed the mbell/replace_travis_ga branch from d42cfa6 to c8e35b3 Compare September 3, 2021 11:40

mjjbell added 2 commits September 3, 2021 12:42

Format unit_tests

1f2bb14

Fix changelog entry

1b87b47

TheMarex merged commit 0911691 into master Sep 3, 2021

TheMarex deleted the mbell/replace_travis_ga branch September 3, 2021 16:35

mjjbell mentioned this pull request Sep 3, 2021

Upgrade ci to xenial #5901

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Travis with Github Actions for CI builds #6071

Replace Travis with Github Actions for CI builds #6071

mjjbell commented Jul 4, 2021

mjjbell Jul 4, 2021

TheMarex Sep 3, 2021

mjjbell Sep 3, 2021

mjjbell Jul 4, 2021 •

edited

Loading

TheMarex Sep 3, 2021

mjjbell Jul 4, 2021

TheMarex Sep 3, 2021

mjjbell Sep 3, 2021

TheMarex Sep 3, 2021

mjjbell commented Jul 8, 2021

mjjbell commented Jul 8, 2021

mjjbell commented Jul 8, 2021

TheMarex commented Sep 1, 2021

TheMarex left a comment

TheMarex Sep 3, 2021

TheMarex Sep 3, 2021

TheMarex Sep 3, 2021

mjjbell Sep 3, 2021

TheMarex Sep 3, 2021

mjjbell commented Sep 3, 2021

mjjbell commented Sep 3, 2021 •

edited

Loading

TheMarex commented Sep 3, 2021

Replace Travis with Github Actions for CI builds #6071

Replace Travis with Github Actions for CI builds #6071

Conversation

mjjbell commented Jul 4, 2021

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjjbell Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjjbell commented Jul 8, 2021

mjjbell commented Jul 8, 2021

mjjbell commented Jul 8, 2021

TheMarex commented Sep 1, 2021

TheMarex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjjbell commented Sep 3, 2021

mjjbell commented Sep 3, 2021 • edited Loading

TheMarex commented Sep 3, 2021

mjjbell Jul 4, 2021 •

edited

Loading

mjjbell commented Sep 3, 2021 •

edited

Loading